No response compression when there is a referer by kobelb · Pull Request #47751 · elastic/kibana

kobelb · 2019-10-09T19:33:39Z

When there is a "referer", this disables response compression.

kobelb · 2019-10-09T19:35:18Z

src/core/server/http/http_server.ts

+      throw new Error('Server is not created yet');
+    }
+
+    this.server.ext('onRequest', (request, h) => {


I wasn't able to use registerOnPreAuth here because it doesn't expose the info. I'm assuming this generally isn't something we want to support, so I'm using the HapiJS interfaces directly. If you all think we should modify the KibanaRequest to include this, just let me know.

kobelb · 2019-10-09T19:36:32Z

src/core/server/http/http_server.test.ts

+    registerRouter(router);
+
+    await server.start();
+    const response = await innerServer.inject({


I had to use .inject here, as opposed to how the other tests are using supertest with the listener or else I wasn't able to use response.request.info. I also wasn't able to look at response.headers['content-encoding'] because this isn't being set by Hapi...

I don't feel great about testing implementation details rather than the actual HTTP response. It looks like supertest allows setting headers with the set method. Is it overriding it? If so, could we use a different way of sending a request in this test?

I don't either... Even if I set the request headers, the response coming back from HapiJS doesn't have the Content-Encoding header set, even though it ends up having it in the functional tests...

HapiJS seems to be skipping the actual compression in this situation, and it's not immediately obvious why: https://github.com/elastic/kibana/pull/47751/files#diff-51730cbab06a430a940f78fc9e710bd7R620-R637

Hmm, maybe not worth figuring out why since we do at least the functional test which covers the blackbox behavior.

I can look into it further to try to understand why. I gave up more easily than I usually do because.... hapi...

elasticmachine · 2019-10-09T21:19:12Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: cea9639

joshdover · 2019-10-10T20:43:09Z

src/core/server/http/http_server.test.ts

+    registerRouter(router);
+
+    await server.start();
+    const response = await innerServer.inject({


Hmm, maybe not worth figuring out why since we do at least the functional test which covers the blackbox behavior.

mshustov · 2019-10-18T14:13:09Z

src/core/server/http/http_server.test.ts

+
+    const router = new Router('', logger, enhanceWithContext);
+    router.get({ path: '/', validate: false }, (context, req, res) =>
+      res.ok({ body: 'hello', headers: { 'Content-Type': 'text/html; charset=UTF-8' } })


I believe there is threshold 1kB to enable gzip. Should fix the problem:

body: 'hello'.repeat(500),

Thanks Mikhail!

mshustov · 2019-10-18T14:14:09Z

src/core/server/http/http_server.test.ts

+    await supertest(innerServer.listener)
+      .get('/')
+      .set('accept-encoding', 'gzip')
+      .then(response => {


can we avoid mixing async/await and promise syntax?

const response = await supertest(...);

It's rather prevalent within this file, but sure!

mshustov

Let's switch to http interface instead of using hapi specific API.

kobelb · 2019-10-18T14:19:40Z

Let's switch to http interface instead of using hapi specific API.

To be able to use registerOnPreAuth, I'll have to modify KibanaRequest to be able to get request.info.referrer and set request.info.acceptEncoding. Unless there's an alternative approach that I haven't thought of. Is that acceptable?

mshustov · 2019-10-18T14:22:12Z

@kobelb sorry, I mean for tests. implementation is ok

…th-referer

elasticmachine · 2019-10-18T19:34:55Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 4e3830d

mshustov · 2019-10-18T19:56:43Z

src/core/server/http/http_server.test.ts

+
+    const router = new Router('', logger, enhanceWithContext);
+    router.get({ path: '/', validate: false }, (context, req, res) =>
+      res.ok({ body: 'hello'.repeat(500), headers: { 'Content-Type': 'text/html; charset=UTF-8' } })


shall we add a comment why we make such a long body?

Yup, great idea. I will make it so.

elasticmachine · 2019-10-18T21:37:38Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 607d460

elasticmachine · 2019-10-18T23:42:44Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 4ad1073

mshustov · 2019-10-21T06:46:59Z

@elasticmachine merge upstream

elasticmachine · 2019-10-21T07:59:17Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 2eded7b

* Hacking it in there, this is obviously not where this belongs * Moving implementation to a private method * Adding unit tests, I don't like the way I had to write these * Adding integration tests * Test not relying on implementation details... * No longer using .inject, thanks Mikhail!!! * Adding comment explaining the long body * Fixing nesting of describes for api integration tests

This reverts commit 85e5885.

…9987) This reverts commit 85e5885.

…" (elastic#49987) This reverts commit 85e5885.

…9987) (#50064) This reverts commit 85e5885.

…9987) (#50065) This reverts commit 85e5885.

kobelb added 4 commits October 9, 2019 09:36

Hacking it in there, this is obviously not where this belongs

a0924c0

Moving implementation to a private method

fee97e4

Adding unit tests, I don't like the way I had to write these

bc766e6

Adding integration tests

bf7029e

kobelb requested a review from a team as a code owner October 9, 2019 19:33

kobelb commented Oct 9, 2019

View reviewed changes

kobelb added the release_note:skip Skip the PR/issue when compiling release notes label Oct 9, 2019

Test not relying on implementation details...

cea9639

kobelb added v8.0.0 v7.5.0 labels Oct 9, 2019

joshdover approved these changes Oct 10, 2019

View reviewed changes

mshustov reviewed Oct 18, 2019

View reviewed changes

mshustov suggested changes Oct 18, 2019

View reviewed changes

kobelb added 2 commits October 18, 2019 07:43

Merge remote-tracking branch 'upstream/master' into no-compression-wi…

662dd29

…th-referer

No longer using .inject, thanks Mikhail!!!

4e3830d

mshustov reviewed Oct 18, 2019

View reviewed changes

mshustov approved these changes Oct 18, 2019

View reviewed changes

Adding comment explaining the long body

607d460

Fixing nesting of describes for api integration tests

4ad1073

Merge branch 'master' into no-compression-with-referer

2eded7b

kobelb merged commit 85e5885 into elastic:master Oct 21, 2019

kobelb deleted the no-compression-with-referer branch October 21, 2019 13:35

kobelb added the v7.6.0 label Oct 21, 2019

kobelb mentioned this pull request Oct 21, 2019

[7.x] No response compression when there is a referer (#47751) #48768

Merged

kobelb mentioned this pull request Oct 21, 2019

[7.5] No response compression when there is a referer (#47751) #48769

Merged

kobelb added a commit to kobelb/kibana that referenced this pull request Nov 1, 2019

Revert "No response compression when there is a referer (elastic#47751)"

d66231d

This reverts commit 85e5885.

kobelb added a commit that referenced this pull request Nov 6, 2019

Revert "No response compression when there is a referer (#47751)" (#4…

c4d1430

…9987) This reverts commit 85e5885.

This was referenced Nov 6, 2019

[7.x] Revert "No response compression when there is a referer (#47751)" (#49987) #50064

Merged

[7.5] Revert "No response compression when there is a referer (#47751)" (#49987) #50065

Merged

kobelb added a commit to kobelb/kibana that referenced this pull request Nov 6, 2019

Revert "No response compression when there is a referer (elastic#47751)…

a2d6518

…" (elastic#49987) This reverts commit 85e5885.

kobelb added a commit that referenced this pull request Nov 7, 2019

Revert "No response compression when there is a referer (#47751)" (#4…

e21a104

…9987) (#50064) This reverts commit 85e5885.

kobelb added a commit that referenced this pull request Nov 7, 2019

Revert "No response compression when there is a referer (#47751)" (#4…

9d93f1f

…9987) (#50065) This reverts commit 85e5885.

jportner mentioned this pull request Nov 14, 2019

Http compression config #50738

Merged

Conversation

kobelb commented Oct 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Oct 9, 2019

💔 Build Failed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mshustov left a comment

Choose a reason for hiding this comment

Uh oh!

kobelb commented Oct 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mshustov commented Oct 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Oct 18, 2019

💔 Build Failed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Oct 18, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Oct 18, 2019

💚 Build Succeeded

Uh oh!

mshustov commented Oct 21, 2019

Uh oh!

elasticmachine commented Oct 21, 2019

💚 Build Succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kobelb commented Oct 18, 2019 •

edited

Loading

mshustov commented Oct 18, 2019 •

edited

Loading